Let's add a comment that closeSpan does not note any changes to the .current export and that the caller is in charge of that. An alternative would be to always note a deletion of that export when closing, but that's unnecessary churn for the common case.

Done.

Why use ReturnType here instead of Promise<T> ?

No strong reason, just a way to emphasize that the happy-path result is just decoration of the function.

I think addCleanup should be allowed to accept a function that returns something else than a promise, but the problem is that Promise<void> | void has undesirable behavior. Maybe Promise<any> | any, just for documentation purposes?

I think it'd be easier to just wrap with async () => { … }, but we can cross that bridge if/when we get there. This is an internal async helper, and there's no current need for supporting non-async callbacks.

Consider extracting the asyncification in a standalone refactor commit.

It actually started that way, but it turned out that only two of the changes were non-fundamental.

What do you mean by "non-fundamental". I consider making a function and all its callers transitively async to be a pure refactor.

Do you mean that you want to see a commit which is just marking functions as async and inserting await at their call sites, with no point-in-time motivation for such a refactor until the following commit adds internal awaits, as opposed to a42c45f including both? I don't think I would find that useful.

That would be my soft preference, yes. It's similar to moving some code around in a first commit in preparation for a subsequent commit which has a more minimal behavioral change.

Consider extracting the rename of path and tmp in a standalone refactor commit.

Given that the swingStore does extra / different work when archiveTranscript is present, should this be a variation of the test?

Same for archiveSnapshot, especially if we end up teeing the compression stream.

The work is purely additive, so I figured it was fine to always check in these tests rather than multiply its configuration to the cross product of keepTranscripts={true,false} and archiveTranscript={undefined,fn}—plus there's lots of testing that covers initSwingStore with archiveTranscript=undefined, including in this file. Do you feel strongly about making this a test variation?

Not strongly no, but since we already have a macro, I thought it could be cheap enough. Not all permutations need to be tested (e.g. transcript options are fully independent of the snapshot option)

Why use the sync version if we're in an async function ?

No strong reason; this step doesn't need to be async and honestly wouldn't benefit from it anyway.

I'm just wondering because I believe it means the removeCallback is sync, which may be unexpected (according to types) for the cleanup. I do remember that tmp had some surprising changes in behavior between sync and async, but I'm not sure what those where now.

This is necessary to preserve the log ordering in terminate.test.js. Alternatively (or additionally), we could update bootstrap-terminate-with-presence.js to tolerate ordering variation.

I am confused about why the simple await was not working.

Figuring it out was an interesting dive... vat-vat-admin.js terminateWithFailure ends with a synchronous call to D(vatAdminDev).terminateWithFailure, which through further synchronous calls in device-vat-admin.js → kernel/deviceManager.js → kernel.js → kernelSyscall.js → vat-admin-hooks.js
invokes void terminateVat(vatID, true, marshalledReason), which means that only the synchronous prelude of terminateVat has an effect before the result promise of E(vatAdminNode).terminateWithFailure(reason) settles and enqueues notifies.

Before this PR, kernelKeeper.removeVatFromSwingStoreExports was synchronous and therefore could not impede the following step's rejection of promises yet to be decided by the terminating vat (which therefore preceded fulfillment of the terminateWithFailure result promise).

But now that this PR is making removeVatFromSwingStoreExports async, a naïve await thereof would end the synchronous prelude before dead promise rejection, which is what caused the assertion in terminate.test.js to fail. Deferring the await restores the expected ordering, although we may want to move it even further down to avoid bumping following logic out of the synchronous prelude, e.g.:

    if (critical) {
      // The following error construction is a bit awkward, but (1) it saves us
      // from doing unmarshaling while in the kernel, while (2) it protects
      // against the info not actually encoding an error, but (3) it still
      // provides some diagnostic information back to the host, and (4) this
      // should happen rarely enough that if you have to do a little bit of
      // extra interpretive work on the receiving end to diagnose the problem,
      // it's going to be a small cost compared to the trouble you're probably
      // already in anyway if this happens.
      panic(`critical vat ${vatID} failed`, Error(info.body));
    } else if (vatAdminRootKref) {
      // static vat termination can happen before vat admin vat exists
      notifyTermination(
        vatID,
        vatAdminRootKref,
        shouldReject,
        info,
        queueToKref,
      );
    } else {
      console.log(
        `warning: vat ${vatID} terminated without a vatAdmin to report to`,
      );
    }

    // worker, if present, needs to be stopped (note that this only applies to ephemeral vats)
    deferred.push(() => vatWarehouse.stopWorker(vatID));

    await Promise.all(deferred);

I actually would want to go a step further and remove the async nature of this function, but still make it return a promise. However that would release zalgo somewhat, so we should not do that in a hurry now.

Please go ahead with the proposed change above, but use PromiseAllOrErrors to avoid swallowing multiple errors.

Filed #10073 to track addressing this footgun

Oh, this makes me sad. Swingstore wants to be synchronous for all the "normal" day-to-day operations. I'm ok with async for commit and exports and things done by the host, but I'd really prefer that everything the kernel touches is synchronous.

I see that it happened to fit into the existing callers (they're already awaiting a snapshot write, or a vat eviction), so the contagion didn't have a big impact, but in general, any async operational API call raises concerns about what state the swingstore is in while that Promise is pending.

In this case, there's a period where the old span is closed, but the new span has not yet been created (we're sitting in the await closeSpan(), which is sitting in its await archiveTranscript). If someone calls into swingstore while that Promise is pending, we'll have no isCurrent spans, and doing something like addItem will throw (at best) or be very confused (at worst). With a synchronous interface, this invariant-violating state is not exposed.

As you know, I'm not a fan of async file IO, I'm not convinced that there's enough perf improvement (if any) to justify the complexity. But I do see that you didn't have a lot of choice.. the gzip streams used by makeArchiveTranscript would be a PITA to use synchronously (but that just makes me sad about the gzip APIs we have available).

So, ok, but let's make it a goal to keep the API synchronous, and only allow exceptions like this when really necessary.

(sorry I missed this when the review was pending, I was OOO).

No worries; after the fact reviews are still much appreciated! Should we open a new ticket about refactoring to isolate asynchronicity?